-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[Fix] destructuring-assignment
: Handle destructuring of useContext in SFC
#2797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…in SFC Co-authored-by: Jin Young Park <[email protected]> Co-authored-by: Jordan Harband <[email protected]>
const destructuringUseContext = destructuring && node.init.callee && node.init.callee.name === 'useContext'; | ||
// let foo = useContext(aContext); | ||
const assignUseContext = identifier && node.init.callee && node.init.callee.name === 'useContext'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure useContext
is the react hook? I think we should make sure that nobody's using a custom hook or function named useContext
, and that this useContext
is imported/required from react
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://reactjs.org/docs/hooks-reference.html#usecontext
here in official docs of react says useContext
is one of basic hooks with useState
, useEffect
.
and we import useContext
like import {useContext} from 'react'
;
I think if somebody's using function named useContext
which overwrite basic hooks , I think we should make another rule about custom hooks which overwrite basic hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone might be using a version of React that doesn't have hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then is there any possible way to check version of react? or we should not include this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Check out lib/util/version.js
's testReactVersion
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I have to import testReactVersion
method in lib/rules/destructuring-assignment'
to check react version in rules? I have no idea.. could you help me with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right
@Zinyon are you still interested in completing this PR? |
The |
@ljharb sure, i will look around |
59af733
to
865ed16
Compare
069314a
to
181c68f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2797 +/- ##
=======================================
Coverage 97.58% 97.58%
=======================================
Files 130 130
Lines 9223 9223
Branches 3349 3349
=======================================
Hits 9000 9000
Misses 223 223 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I went ahead and added the react version dependence; i'll leave "make sure it's destructured from the pragma" as a followup. |
i reopen PR on resolve issue - #2309 cause i did a lot of commits while editing.
Fixes #2309. Closes #2787.